Skip to content

Conversation

@brandon-pereira
Copy link
Member

Improves user reported issues that clicking outside the modal would click elements unexpectedly, also improves accessibility (keyboard focus trap & esc to exit)

Fixes HDX-2642

Improves user reported issues that clicking outside the modal would click elements unexpectedly, also improves accessibility (keyboard focus trap & esc to exit)

Fixes HDX-2642
@brandon-pereira brandon-pereira requested review from a team and pulpdrew and removed request for a team October 23, 2025 14:45
@changeset-bot
Copy link

changeset-bot bot commented Oct 23, 2025

🦋 Changeset detected

Latest commit: 39d2805

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@hyperdx/app Patch
@hyperdx/api Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel
Copy link

vercel bot commented Oct 23, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
hyperdx-v2-oss-app Ready Ready Preview Comment Oct 27, 2025 2:46pm

@github-actions
Copy link
Contributor

github-actions bot commented Oct 23, 2025

E2E Test Results

All tests passed • 25 passed • 3 skipped • 261s

Status Count
✅ Passed 25
❌ Failed 0
⚠️ Flaky 1
⏭️ Skipped 3

View full report →

@claude
Copy link

claude bot commented Oct 23, 2025

PR Review: Drawer Migration to Mantine

Overall assessment: Good migration from react-modern-drawer to Mantine Drawer with improved accessibility.

Critical Issues

  • ⚠️ Missing trapFocus prop → Add trapFocus={true} to all Drawer components for proper keyboard focus trap (DBRowSidePanel.tsx:505, SessionSidePanel.tsx:74, and all other Drawer instances)
  • ⚠️ Missing closeOnEscape → Add closeOnEscape={true} explicitly for consistency (currently relying on manual useHotkeys in some places)
  • ⚠️ Inconsistent overlay behavior → Old code had enableOverlay={subDrawerOpen} logic that's been removed - verify nested drawers still work correctly with overlay

Code Quality Issues

  • 🔍 DBRowSidePanel useClickOutside hook (line 498-502) → Mantine Drawer has built-in closeOnClickOutside prop, consider using it instead of manual hook to reduce complexity
  • 🔍 Popover portalProps (DBRowTableFieldWithPopover.tsx:156) → Changed from zIndex={1} to portalProps={{target: tableContainerRef?.current}} - verify popovers render correctly within drawers and don't get clipped

Testing Required

  • Test nested drawer scenarios (e.g., opening a row panel from a pod details panel)
  • Verify Esc key closes drawers properly
  • Verify clicking outside closes drawers but doesn't trigger clicks underneath
  • Test keyboard navigation and focus trap within drawers

Minor Notes

  • ✅ Good: Removed unused react-modern-drawer dependency
  • ✅ Good: Updated E2E tests to use data-testid
  • ✅ Good: Consistent styling patterns across all drawer migrations

Copy link
Contributor

@pulpdrew pulpdrew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kodiakhq kodiakhq bot merged commit bb3539d into main Oct 27, 2025
9 checks passed
@kodiakhq kodiakhq bot deleted the brandon/drawer-focus-trap branch October 27, 2025 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants